chore(dot/state/pruner): refactoring and fixes #2831
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## development #2831 +/- ##
===============================================
+ Coverage 51.57% 52.17% +0.59%
===============================================
Files 219 223 +4
Lines 27984 28124 +140
===============================================
+ Hits 14434 14674 +240
+ Misses 12267 12204 -63
+ Partials 1283 1246 -37 |
qdm12
force-pushed
the
qdm12/state/pruner/refactor
branch
from
85c570d
to
329da6a
Compare
qdm12
force-pushed
the
qdm12/state/pruner/refactor
branch
2 times, most recently
from
725533d
to
667cd1f
Compare
qdm12
force-pushed
the
qdm12/state/pruner/refactor
branch
4 times, most recently
from
6f17141
to
adec1e3
Compare
12 tasks
qdm12
force-pushed
the
qdm12/state/pruner/refactor
branch
from
adec1e3
to
e524268
Compare
qdm12
force-pushed
the
qdm12/state/pruner/refactor
branch
5 times, most recently
from
a1819b1
to
6d61bd8
Compare
qdm12
requested review from
noot,
edwardmack,
timwu20,
EclesioMeloJunior,
jimjbrettj and
kishansagathiya
as code owners
qdm12
force-pushed
the
qdm12/state/pruner/refactor
branch
from
260cc25
to
ce064cf
Compare
qdm12
commented
Jan 6, 2023
Comment on lines
-31
to
+32
| // DefaultPruningMode is the default pruning mode | ||
| DefaultPruningMode = "archive" | ||
| // DefaultPruningEnabled describes if online pruning should be enabled by default. | ||
| DefaultPruningEnabled = false |
There was a problem hiding this comment.
Let me know if you prefer to keep it as a string (might be needed for more pruning options later), but I changed it to a boolean to keep it simpler for now.
qdm12
changed the title
qdm12
force-pushed
the
qdm12/state/pruner/refactor
branch
from
ce064cf
to
194f1f2
Compare
qdm12
commented
Jan 6, 2023
Comment on lines
-159
to
-160
| // storePruningData stores the pruner configuration. | ||
| func (s *BaseState) storePruningData(mode pruner.Config) error { |
There was a problem hiding this comment.
That was persisting the pruning mode, whereas really it should be given at start from the toml config or flags, and be able to change from one mode to the other.
Comment on lines
+79
to
+81
| // Temporary work-around until we drop merkle values for node hashes in another PR (already opened). | ||
| insertedNodeHashes := make(map[common.Hash]struct{}, len(insertedMerkleValues)) | ||
| for k := range insertedMerkleValues { |
There was a problem hiding this comment.
Conversion will be removed once #2915 gets merged.
Comment on lines
+428
to
+433
| if dstv.IsNil() { | ||
| keyType := dstv.Type().Key() | ||
| valueType := dstv.Type().Elem() | ||
| aMapType := reflect.MapOf(keyType, valueType) | ||
| dstv.Set(reflect.MakeMapWithSize(aMapType, int(numberOfTuples))) | ||
| } |
There was a problem hiding this comment.
this is so it can be decoded to a nil typed destination map. I don't think it's needed so much anymore but it was when I had a struct with map fields (and it sucks to init all maps)
qdm12
force-pushed
the
qdm12/state/pruner/refactor
branch
from
194f1f2
to
ad3101e
Compare
qdm12
commented
Jan 13, 2023
| logger.EXPECT().Debugf("journal data stored for block number %d and block hash %s", | ||
| uint32(1), "0x66000000...00000000") | ||
| err = pruner.RecordAndPrune( | ||
| map[common.Hash]struct{}{{3}: {}}, // deleted node hashes |
There was a problem hiding this comment.
Should we prune 3? How should we handle forks??
EDIT: we should handle forks, this is WIP
qdm12
commented
Jan 26, 2023
| @@ -176,9 +175,9 @@ func TestService_StorageTriePruning(t *testing.T) { | |||
| config := Config{ | |||
There was a problem hiding this comment.
The test TestService_StorageTriePruning which was modified in #3063 should now be unskipped.
- Move dot/state/pruner to internal/pruner/archive and internal/pruner/full - Define pruner as dot/state local interface - Inject pruner in NewStorageState - Unexport newStorageState - `NewArchiveNode` constructor - Change pruner config from `Mode` to bool - It's either on or off really - Change `--pruning` to be a boolean flag - Change pruning toml field to be a boolean - Pruner config changes - Do not store and load pruning config from base state - Move Config struct definition in `dot/state` - Set pruning state service config from user config in `dot` - Database batches for storing journal records - Unit and integration tests - Rename `StoreJournalRecord` to `RecordAndPrune` - Genesis block: no journal data and no pruning
qdm12
force-pushed
the
qdm12/state/pruner/refactor
branch
from
ad3101e
to
ed2de43
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
internal/pruner/fullandinternal/pruner/archivenode hash+common.Hashinstead ofmerkle value+string, since it's only 32B node hashesNewIteratordatabase method from production codeStoreJournalRecordtoRecordAndPruneTests
Issues
Fixes #2835
Primary Reviewer
@EclesioMeloJunior